Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update(config): Improve http output, add compression and keep_alive #2974

Merged
merged 2 commits into from
Dec 18, 2023
Merged

update(config): Improve http output, add compression and keep_alive #2974

merged 2 commits into from
Dec 18, 2023

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Dec 16, 2023 •
edited
Loading

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

This PR adds two new configuration options for the http output:

  • Post compression
  • Connection keep alive
  • Batching

Which issue(s) this PR fixes:

Fixes #2955

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat(userspace/falco): falco administrators can now configure the http output to compress the data sent as well as enable keep alive for the connection. Two new fields (compress_uploads and keep_alive) in the http_output block of the `falco.yaml` file can be used for that purpose. Both are disabled by default.

@poiana poiana requested review from incertum and Kaizhe December 16, 2023 13:45
@poiana poiana added the size/S label Dec 16, 2023
@sgaist
feat(outputs_http): implement support for compressed upload
ab7f69d 
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
@sgaist
feat(outputs_http): implement keep alive
36c7848 
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
@sgaist sgaist force-pushed the improve_http_output branch from ef96640 to 36c7848 Compare December 16, 2023 13:49
incertum
incertum reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch @sgaist 🚀 !

Recommend not marking this as feat and instead as new feature that doesn't break the old default behavior. More thoughts?

CC @leogr @Issif

/milestone 0.37.0

@poiana poiana added this to the 0.37.0 milestone Dec 16, 2023
@FedeDP
Copy link
Contributor

FedeDP commented Dec 18, 2023

Recommend not marking this as feat and instead as new feature that doesn't break the old default behavior. More thoughts?

What do you mean?
A feat does not necessarily impose a breaking change. In this case, no breaking change in the default behavior is to be expected, right?
I think we are already ok with this one.

FedeDP
FedeDP approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana poiana added the lgtm label Dec 18, 2023
@poiana
Copy link
Contributor

poiana commented Dec 18, 2023

LGTM label has been added.

Git tree hash: 1c103b6386c1efb7bbb25b90049771303ae62101

@incertum
Copy link
Contributor

/hold I think @sgaist is still looking into possibly adding batching 🙃

@sgaist
Copy link
Contributor Author

sgaist commented Dec 18, 2023

@incertum I am currently waiting on an answer with regard to the batching part which will likely require a bit more work and design discussions.

What could be done is to split things and implement said batching in a follow up patch so the benefits of compression and keep alive are already made available. This would only require a bit of rewording of the pull request message.

@incertum incertum changed the title Improve http output update(config): Improve http output, add compression and keep_alive Dec 18, 2023
incertum
incertum approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

/unhold

Yes let's split PRs!

@poiana
Copy link
Contributor

poiana commented Dec 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, sgaist

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit d99c137 into falcosecurity:master Dec 18, 2023
28 checks passed
@Issif
Copy link
Member

Issif commented Dec 18, 2023 •
edited
Loading

Don't know if the batching will be really useful, very few targets accept that. On the other hands, having more control on the http output would be a really nice thing:

  • allow to set the User-Agent
  • allow to set the Headers (really useful for Authentications either)
  • allow to set the Http Method (PUT/POST)
  • allow to use mTLS for the authentication

@sgaist
Copy link
Contributor Author

sgaist commented Dec 19, 2023

@Issif based on the current code of the http output, the following are already available:

  • User agent setting
  • mTLS configuration

When you say authentication, are you thinking about setting a username/password or a token in the authorization header or something more involved like oauth ?

@sgaist sgaist deleted the improve_http_output branch December 19, 2023 08:28
@sgaist sgaist mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum approved these changes

@FedeDP FedeDP FedeDP approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@FedeDP FedeDP

@incertum incertum

Labels
approved area/engine dco-signoff: yes kind/feature lgtm release-note size/S
Projects
Falco Roadmap
Archived in project
Milestone
0.37.0
Development

Successfully merging this pull request may close these issues.

new http_output options (compression, batching, keep_alive)
5 participants
@sgaist @FedeDP @poiana @incertum @Issif